Skip to content

fix/batch-changes: validate files target paths in changeset hooks and executor#1352

Open
cbrnrd wants to merge 1 commit into
mainfrom
carterbrainerd-vuln-104-changesethooks-files-validation-bypass-can-reach-executor
Open

fix/batch-changes: validate files target paths in changeset hooks and executor#1352
cbrnrd wants to merge 1 commit into
mainfrom
carterbrainerd-vuln-104-changesethooks-files-validation-bypass-can-reach-executor

Conversation

@cbrnrd

@cbrnrd cbrnrd commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Problem

src-cli rejects commas in top-level steps.files target paths because a comma breaks out of Docker --mount syntax and lets an attacker inject arbitrary source=/target= pairs (e.g. mounting /var/run/docker.sock). That same validation was missing for changesetHooks.*.steps[].files, so a malicious v3 batch spec could smuggle a dangerous files target path through a hook and reach the executor-side Docker mount sink.

The executor (src batch exec) makes this worse: it loads already-parsed Step structs from JSON and never re-runs ParseBatchSpec/validateHooks, so parse-time validation alone is a coordinator-only guard.

Solution

  • Add the missing comma check for files target paths in validateHooks (covers both onCIFailure and onMergeConflict).
  • Add defense-in-depth validation at the executor sink in createFilesToMount, so the check holds regardless of how a step reaches the executor.

Verification Evidence

$ go test ./internal/batches/service/ -run ParseBatchSpec -v
--- PASS: TestService_ParseBatchSpec/hook_files_target_path_with_comma

$ go test ./internal/batches/executor/ -run CreateFilesToMount -v
--- PASS: TestCreateFilesToMount_RejectsCommaInTargetPath

$ go test ./internal/batches/...
ok  	github.com/sourcegraph/src-cli/internal/batches/executor	2.402s
ok  	github.com/sourcegraph/src-cli/internal/batches/service	0.626s
(all batch packages pass)

Comment thread lib/batches/batch_spec.go
)))
}
}
for name := range step.Files {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI the code here is vendored from our monorepo.

Comment on lines +543 to +552
// Defense-in-depth: the mount target name is concatenated directly
// into a Docker `--mount type=bind,...,target=<name>` argument. A comma
// in the name would let an attacker break out of the mount spec and
// inject arbitrary source=/target= pairs (e.g. /var/run/docker.sock).
// Parse-time validation rejects this, but the executor receives
// pre-parsed steps via JSON and never re-runs that validation, so we
// re-check here at the sink.
if strings.Contains(name, ",") {
return nil, cleanup, errors.Newf("files target path %q contains invalid characters", name)
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok so this would of failed which is what we want, but now we fail earlier? And that is better from both a UX perspective + extra defense incase the other check for some reason doesn't run?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants